-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH]: add batch get version file paths method to Sysdb #4432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH]: add batch get version file paths method to Sysdb #4432
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
ab87d31
to
7e6889e
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
7e6889e
to
ea31ef5
Compare
ea31ef5
to
4d29f0d
Compare
4d29f0d
to
0d136b9
Compare
Add Batch Get Collection Version File Paths to SysDB API This PR introduces a new batch API to retrieve version file paths for multiple collections in SysDB, which is necessary for operations like garbage collecting collections in fork trees. The change spans both Rust and Go components, defining new proto messages and service endpoints, implementing the server, coordinator, and DAO methods, providing tests, and adding error handling for batch retrieval. Key Changes: Affected Areas: Potential Impact: Functionality: Enables efficient retrieval of version file paths for batched collection IDs, improving garbage collection workflows. No breaking changes to existing APIs. Performance: Batch fetch is more efficient for clients. Current Go implementation uses IN queries, which may have DB/backend limitations for very large batches (see code TODO). Security: No direct security impact; assumes existing API authentication applies. Scalability: Scales better than single-fetch-per-collection but might bottleneck on database IN queries for very large batch sizes; further optimization noted as a TODO. Review Focus: Testing Needed• Verify the batch Code Quality Assessmentgo/pkg/sysdb/metastore/db/dao/collection.go: Batch query is direct, uses standard GORM patterns but flagged by reviewer 'todo: make more efficient'. go/pkg/sysdb/metastore/db/dbmodel/mocks/ICollectionDb.go: Regenerated, OK. go/pkg/sysdb/coordinator/coordinator_test.go: New test covers positive path; well integrated. rust/sysdb/src/sysdb.rs: New method added cleanly, uses appropriate Rust error handling and enum patterns. Some branches are TODO (Sqlite). rust/sysdb/src/test_sysdb.rs: Extended with set/get functionality and error coverage. Logic is straightforward. go/pkg/sysdb/coordinator/table_catalog.go: Implements batch method directly; review notes touches on exposure of proto types. Best PracticesAPI Design: Testing: Error Handling: Separation Of Concerns: Potential Issues• Potential performance bottleneck for large batch sizes in the DAO's SQL IN queries This summary was automatically generated by @propel-code-bot |
93be17d
to
ea366a5
Compare
b39fd56
to
8a3421d
Compare
ea366a5
to
edf6743
Compare
8a3421d
to
972427e
Compare
505c4ae
to
3ece42b
Compare
972427e
to
bab5b68
Compare
3ece42b
to
5801be6
Compare
5801be6
to
90ac80a
Compare
bab5b68
to
5f7fa4a
Compare
90ac80a
to
2e26e8a
Compare
5f7fa4a
to
71c953d
Compare
2e26e8a
to
7344e5c
Compare
c3a7502
to
2425e04
Compare
2425e04
to
a09392f
Compare
a09392f
to
1af74ec
Compare
Merge activity
|
Description of changes
Needed when garbage collecting a collection in a fork tree, because we need to read the version files of all other nodes in the tree.
Test plan
How are these changes tested?
Added a test.
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?
n/a